fix: search input initialization and re-focus bug#2148
fix: search input initialization and re-focus bug#2148nnecec wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPreserve client-side focused global search input across hydration and routing: initialise searchQuery from focused input, restrict URL→state syncing to the search route and avoid clobbering active input, reset/focus home search on index navigation, and perform one-time hydration recovery from focused input. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DOM as "Browser / focused <input>"
participant Composable as "useGlobalSearch"
participant Router as "Route / query"
participant Render as "nextTick / Render"
User->>DOM: focus / type in search input
DOM->>Composable: getFocusedSearchInputValue() (client-only)
Composable->>Composable: initialise searchQuery from focused input (client)
Note over Router,Composable: watcher triggers on [route.name, route.query.q]
Router->>Composable: route.name changes
alt route.name == 'search' and no input focused
Router->>Composable: apply normalized query → searchQuery (only if different)
else input is focused
Composable-->>Router: skip clobbering searchQuery
end
opt route.name becomes 'index' (client)
Composable->>Composable: clear searchQuery & committedSearchQuery
Composable->>Render: nextTick
Render->>DOM: focus and select `#home-search`
end
Note over DOM,Composable: onMounted hydration: if searchQuery empty and focused input has value, set via computed setter to restore instant-search.
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/composables/useGlobalSearch.ts (1)
60-67: Consider extracting focused input detection into a reusable helper.The logic for detecting whether a search input is focused (lines 60-67) duplicates the pattern from
getFocusedSearchInputValue(lines 22-24). Extracting a smallisSearchInputFocused()helper would improve maintainability.♻️ Proposed refactor
+const isSearchInputFocused = (): boolean => { + if (!import.meta.client) return false + const active = document.activeElement + if (!(active instanceof HTMLInputElement)) return false + return active.type === 'search' || active.name === 'q' +} + const getFocusedSearchInputValue = () => { - if (!import.meta.client) return '' - - const active = document.activeElement - if (!(active instanceof HTMLInputElement)) return '' - if (active.type !== 'search' && active.name !== 'q') return '' - return active.value + if (!isSearchInputFocused()) return '' + return (document.activeElement as HTMLInputElement).value }Then in the watcher:
- if (import.meta.client) { - const active = document.activeElement - if ( - active instanceof HTMLInputElement && - (active.type === 'search' || active.name === 'q') - ) { - return - } - } + if (isSearchInputFocused()) return
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecd4a617-b5c7-4a12-93b5-29da7a115b93
📒 Files selected for processing (1)
app/composables/useGlobalSearch.ts
399f570 to
4636625
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6ac65a2-c31a-42f4-80dc-a8f836ca6980
📒 Files selected for processing (1)
app/composables/useGlobalSearch.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/composables/useGlobalSearch.ts (2)
65-73:⚠️ Potential issue | 🟠 MajorDo not permanently drop focused URL synchronisation.
This early return also ignores legitimate
/search?q=...history or programmatic changes while the input stays focused, leavingsearchQueryandcommittedSearchQuerystale. Please distinguish self-initiated pending URL updates from external route changes, or queue a resync when the focused input blurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useGlobalSearch.ts` around lines 65 - 73, The current early return in useGlobalSearch that checks document.activeElement prevents handling legitimate external route/query changes; instead of unconditionally returning in the focused-input branch, only suppress updates that are self-initiated (e.g. compare the incoming route.query.q to a pending/last-synced value or to committedSearchQuery) and allow external changes to update searchQuery and committedSearchQuery; if the input is focused and the change is external, queue a resync to apply the new query on blur by attaching a one-time blur handler that will set searchQuery/committedSearchQuery to the route q, and add a small pending flag (or lastSyncedQuery) to differentiate programmatic/self updates from external route changes.
141-174:⚠️ Potential issue | 🟠 MajorGuard mounted recovery after an intentional home reset.
The
/search→/reset can be undone ifonMountedreads the still-focused header input before the cleared model reaches the DOM. Add a one-shot shared guard so the same navigation that clears the home search cannot immediately restore the stale query.Suggested guard shape
+ const skipFocusedRecovery = useState('skip-focused-search-recovery', () => false) + if (import.meta.client) { watch( () => route.name, name => { if (name !== 'index') return + skipFocusedRecovery.value = true searchQuery.value = '' committedSearchQuery.value = '' // Use nextTick so we run after the homepage has rendered. nextTick(() => { @@ if (import.meta.client) { onMounted(() => { + if (skipFocusedRecovery.value && route.name === 'index') { + skipFocusedRecovery.value = false + return + } + const focusedInputValue = getFocusedSearchInputValue() if (!focusedInputValue) return if (searchQuery.value) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useGlobalSearch.ts` around lines 141 - 174, Introduce a one-shot guard ref (e.g., recentlyResetHome) shared between the route watch block and onMounted so the intentional home reset cannot be immediately undone: create a ref flag (recentlyResetHome) default false, set it to true inside the watch handler right when you clear searchQuery/committedSearchQuery (before calling nextTick), and in the onMounted block check if recentlyResetHome.value is true — if so clear the flag and return early instead of restoring from getFocusedSearchInputValue; optionally clear the flag after the nextTick in the watch so it only prevents a single immediate restoration.
🧹 Nitpick comments (1)
app/composables/useGlobalSearch.ts (1)
10-183: Split the client-only search lifecycle concerns.
useGlobalSearchnow owns provider selection, URL sync, debounce control, homepage focus reset, and hydration recovery in one large function. Consider extracting the homepage reset and hydration recovery watchers into small helpers to reduce future state-sync regressions. As per coding guidelines, “Keep functions focused and manageable (generally under 50 lines)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useGlobalSearch.ts` around lines 10 - 183, The useGlobalSearch function is doing too much; extract the homepage-reset watcher and the hydration recovery onMounted logic into two small helper functions to keep responsibilities separated: create (1) resetHomeSearchOnNavigate which contains the watch on route.name that clears searchQuery and committedSearchQuery and focuses/selects the `#home-search` input (using nextTick) and is registered only when import.meta.client, and (2) recoverFocusedInputOnMount which contains the onMounted block that reads getFocusedSearchInputValue and sets searchQueryValue when appropriate; call both helpers from useGlobalSearch in place of the inlined blocks and keep all existing uses of searchQuery, committedSearchQuery, searchQueryValue, getFocusedSearchInputValue, and flushUpdateUrlQuery unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/composables/useGlobalSearch.ts`:
- Around line 21-28: getFocusedSearchInputValue currently returns any focused
search or name="q" input which can capture page-local filters; tighten it to
only return a value for inputs that are explicitly meant to seed the global
search (e.g. check for a marker attribute/class such as data-global-search or
class "global-search-input") and ensure the global search initializer in
useGlobalSearch runs the pagesWithLocalFilter guard before reading any
focused/input value. Update all occurrences of getFocusedSearchInputValue and
the initializer/restore logic in useGlobalSearch (including the other similar
blocks) to use the attribute/class check and to defer reading until after
pagesWithLocalFilter has been checked so local page ?q filters are never
imported into the shared global state.
---
Duplicate comments:
In `@app/composables/useGlobalSearch.ts`:
- Around line 65-73: The current early return in useGlobalSearch that checks
document.activeElement prevents handling legitimate external route/query
changes; instead of unconditionally returning in the focused-input branch, only
suppress updates that are self-initiated (e.g. compare the incoming
route.query.q to a pending/last-synced value or to committedSearchQuery) and
allow external changes to update searchQuery and committedSearchQuery; if the
input is focused and the change is external, queue a resync to apply the new
query on blur by attaching a one-time blur handler that will set
searchQuery/committedSearchQuery to the route q, and add a small pending flag
(or lastSyncedQuery) to differentiate programmatic/self updates from external
route changes.
- Around line 141-174: Introduce a one-shot guard ref (e.g., recentlyResetHome)
shared between the route watch block and onMounted so the intentional home reset
cannot be immediately undone: create a ref flag (recentlyResetHome) default
false, set it to true inside the watch handler right when you clear
searchQuery/committedSearchQuery (before calling nextTick), and in the onMounted
block check if recentlyResetHome.value is true — if so clear the flag and return
early instead of restoring from getFocusedSearchInputValue; optionally clear the
flag after the nextTick in the watch so it only prevents a single immediate
restoration.
---
Nitpick comments:
In `@app/composables/useGlobalSearch.ts`:
- Around line 10-183: The useGlobalSearch function is doing too much; extract
the homepage-reset watcher and the hydration recovery onMounted logic into two
small helper functions to keep responsibilities separated: create (1)
resetHomeSearchOnNavigate which contains the watch on route.name that clears
searchQuery and committedSearchQuery and focuses/selects the `#home-search` input
(using nextTick) and is registered only when import.meta.client, and (2)
recoverFocusedInputOnMount which contains the onMounted block that reads
getFocusedSearchInputValue and sets searchQueryValue when appropriate; call both
helpers from useGlobalSearch in place of the inlined blocks and keep all
existing uses of searchQuery, committedSearchQuery, searchQueryValue,
getFocusedSearchInputValue, and flushUpdateUrlQuery unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25c3d4b8-e75a-43ae-bfc6-b6fc452f845b
📒 Files selected for processing (1)
app/composables/useGlobalSearch.ts
Sorry for missing the comments. I just fixed the question according to coderabbitai's comments. |
…ve search query handling - Moved getFocusedSearchInputValue function to a more appropriate location. - Simplified logic for checking active input element and its type. - Enhanced condition to prevent overwriting search query when focused input matches URL value. - Cleaned up formatting for better readability.
- Added `data-global-search` attribute to search input fields in SearchBox.vue and index.vue to distinguish global search from local filters. - Updated `getFocusedSearchInputValue` function to only capture values from inputs marked with `data-global-search`, enhancing search query handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/composables/useGlobalSearch.ts (1)
169-182: Hydration recovery logic is sound; one small note on duplicated registration.The three-layer guard (
pagesWithLocalFilter→!focusedInputValue→searchQuery.value) correctly prevents both page-local leakage and clobbering the intentional home reset: on/search → /, even ifonMountedfires before theflush: 'post'route watcher,searchQuery.valueis still the pre-reset value and the early return kicks in.One minor observation (applies to both this
onMountedand the watcher at lines 149‑167): becauseuseGlobalSearch()is called by both the layout-levelHeader/SearchBoxand page components (e.g.index.vue), each callsite registers its ownonMounted/route.namewatcher. On navigation to/, the reset-and-refocus path therefore runs twice. It is harmless (idempotent reset, focus on already-focused element) but slightly wasteful — optional to scope via the existingplaceparameter if you want to clean this up later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useGlobalSearch.ts` around lines 169 - 182, Duplicate onMounted/route watcher registrations occur because useGlobalSearch() is invoked from multiple callsites (e.g., Header/SearchBox and page components); to avoid the duplicate reset/refocus path, scope the registration using the existing place parameter (or a per-instance guard) so the onMounted and the route.name watcher only register in one place (e.g., when place === 'header' or place === 'layout'), referencing useGlobalSearch, the onMounted block, the route watcher, pagesWithLocalFilter, and searchQueryValue to locate the code to change; update the conditional so only the intended instance registers the hydration recovery logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/composables/useGlobalSearch.ts`:
- Around line 146-167: Before resetting state in the index route watcher, cancel
any pending debounced tasks to avoid a queued update navigating back to /search:
call updateUrlQuery.cancel() and commitSearchQuery.cancel() (or use the existing
flushUpdateUrlQuery helper if preferred) at the start of the name === 'index'
branch, then proceed to set searchQuery.value and committedSearchQuery.value to
'' and run nextTick to focus the home input.
---
Nitpick comments:
In `@app/composables/useGlobalSearch.ts`:
- Around line 169-182: Duplicate onMounted/route watcher registrations occur
because useGlobalSearch() is invoked from multiple callsites (e.g.,
Header/SearchBox and page components); to avoid the duplicate reset/refocus
path, scope the registration using the existing place parameter (or a
per-instance guard) so the onMounted and the route.name watcher only register in
one place (e.g., when place === 'header' or place === 'layout'), referencing
useGlobalSearch, the onMounted block, the route watcher, pagesWithLocalFilter,
and searchQueryValue to locate the code to change; update the conditional so
only the intended instance registers the hydration recovery logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84dff354-b9d9-4203-ab5f-ffc5f330510f
📒 Files selected for processing (3)
app/components/Header/SearchBox.vueapp/composables/useGlobalSearch.tsapp/pages/index.vue
✅ Files skipped from review due to trivial changes (2)
- app/pages/index.vue
- app/components/Header/SearchBox.vue
2ef9832 to
364c842
Compare
…dates - Enhanced code readability by formatting multi-line function calls and conditions. - Added cancellation for in-flight URL and search query updates to prevent unintended navigation and value restoration.
…tion recovery - Updated logic to register reset/refocus handlers only for the content instance to avoid duplicates when useGlobalSearch is called from multiple locations. - Enhanced hydration recovery by ensuring it only occurs for the content instance, improving performance and consistency.
There was a problem hiding this comment.
The bug can be solved in a simpler way, by adding a ref to the input, and calling its focus onMounted:
const searchInput = ref<HTMLInputElement | null>(null)
onMounted(() => {
searchInput.value?.focus()
})The autofocus attribute can be kept on the input, so it can act as a progressive enhancement.
🔗 Linked issue
no issue
🧭 Context
no context
📚 Description
3.19.1.mp4
Fixed two issues: